Added SpanProcessor OnEnding callback #6367

JonasKunz · 2024-04-09T10:57:11Z

Implementation for the spec change open-telemetry/opentelemetry-specification#4024.

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SpanProcessor.java

codecov · 2024-08-07T10:32:30Z

Codecov Report

Attention: Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.01%. Comparing base (3e8092d) to head (80aa059).
Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
.../main/java/io/opentelemetry/sdk/trace/SdkSpan.java	96.42%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6367      +/-   ##
============================================
- Coverage     90.05%   90.01%   -0.05%     
- Complexity     6276     6330      +54     
============================================
  Files           697      703       +6     
  Lines         18949    19086     +137     
  Branches       1858     1881      +23     
============================================
+ Hits          17065    17180     +115     
- Misses         1312     1331      +19     
- Partials        572      575       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkwatson · 2024-08-13T02:43:07Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

      this.endEpochNanos = endEpochNanos;
-      hasEnded = true;
+      if (spanProcessor.isOnEndingRequired()) {
+        spanProcessor.onEnding(this);


I think it's probably a mistake to be calling this under the lock. I believe we only want to do internal state management while the lock is being held, not call not-under-our-control methods provided by users.

The lock was actually intentionally hold to cover this part of the spec:

The SDK MUST guarantee that the span can no longer be modified by any other thread
before invoking OnEnding of the first SpanProcessor. From that point on, modifications
are only allowed synchronously from within the invoked OnEnding callbacks.

This part was added to the spec so that SpanProcessors can be sure that the state they see the span in is actually consistent and not prone to race conditions due to span modifications from the application code.

However, I now switched to a different way of implementing this, because as you said exposing the lock is probably not the best way of achieving the desired safety here.

# Conflicts: # docs/apidiffs/current_vs_latest/opentelemetry-sdk-trace.txt

…g lock

jack-berg

Couple minor comments, but seems generally good. Thanks!

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/ExtendedSpanProcessor.java

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

jkwatson · 2024-08-22T01:46:45Z

I approve of putting this in a separate interface. But, we should provide documentation on how someone could use this, as well.

JonasKunz · 2024-08-26T08:01:03Z

But, we should provide documentation on how someone could use this, as well.

What do you think would be the best place for this? Just a javadoc in ExtendedSpanProcessor or do we have some markdown files somewhere where this would fit in nicely?

jack-berg · 2024-08-28T19:51:09Z

What do you think would be the best place for this? Just a javadoc in ExtendedSpanProcessor or do we have some markdown files somewhere where this would fit in nicely?

I think the ideal way to do this is a unit test. Let's add an ExtendedSpanProcessorUsageTest akin to what we do in the api incubator. No obvious place to link to the usage test at the moment, but let's start by at least demonstrating its usage in clear terms.

jack-berg

thanks for adding the example. couple of nits but ready otherwise ready to merge IMO

sdk/trace/src/test/java/io/opentelemetry/sdk/trace/internal/ExtendedSpanProcessorUsageTest.java

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

Implemented beforeEnd callback

f257438

JonasKunz mentioned this pull request Apr 9, 2024

Add BeforeEnd to have a callback where the span is still writeable open-telemetry/opentelemetry-specification#1089

Closed

JonasKunz mentioned this pull request Apr 17, 2024

Extended onEnd to allow for explicit calling of next span processor #6394

Closed

pellared mentioned this pull request Jul 4, 2024

Add ability for SpanProcessor to mutate spans on end open-telemetry/opentelemetry-specification#4024

Merged

5 tasks

JonasKunz added 4 commits July 4, 2024 14:21

Rename to onEnding

55f5244

Merge remote-tracking branch 'otel/main' into span-processor-beforeEnd

528727f

Merge remote-tracking branch 'otel/HEAD' into span-processor-beforeEnd

046cbe8

spotless and documentation clarifications

f3fd785

JonasKunz commented Aug 7, 2024

View reviewed changes

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SpanProcessor.java Outdated Show resolved Hide resolved

JonasKunz changed the title ~~[PoC] Added SpanProcessor beforeEnd callback~~ Added SpanProcessor beforeEnd callback Aug 7, 2024

Added jApiCMp output

c7ed970

JonasKunz marked this pull request as ready for review August 7, 2024 11:15

JonasKunz requested a review from a team August 7, 2024 11:15

jkwatson reviewed Aug 13, 2024

View reviewed changes

JonasKunz added 2 commits August 13, 2024 09:34

Merge remote-tracking branch 'otel/HEAD' into span-processor-beforeEnd

2b0c31e

# Conflicts: # docs/apidiffs/current_vs_latest/opentelemetry-sdk-trace.txt

Prevent concurrent modifications from other threads instead of holdin…

d623112

…g lock

JonasKunz changed the title ~~Added SpanProcessor beforeEnd callback~~ Added SpanProcessor OnEnding callback Aug 14, 2024

SylvainJuge mentioned this pull request Aug 14, 2024

add span stacktrace config option open-telemetry/opentelemetry-java-contrib#1414

Merged

JonasKunz added 3 commits August 20, 2024 13:23

Moved onEnding to ExtendedSpanProcessor interface

78e715b

Merge remote-tracking branch 'otel/HEAD' into span-processor-beforeEnd

5e1f295

Fixed string representation test

40f1cef

jack-berg approved these changes Aug 20, 2024

View reviewed changes

JonasKunz and others added 3 commits August 21, 2024 10:21

Apply suggestions from code review

2f686cf

Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

Review fixes

300b44c

Fix javadoc

c3b8784

jack-berg mentioned this pull request Aug 21, 2024

Update javadoc boilerplate for experimental classes which are for public use #6662

Open

Added test case to demonstrate usage

8240cab

jack-berg approved these changes Aug 30, 2024

View reviewed changes

sdk/trace/src/test/java/io/opentelemetry/sdk/trace/internal/ExtendedSpanProcessorUsageTest.java Outdated Show resolved Hide resolved

sdk/trace/src/test/java/io/opentelemetry/sdk/trace/internal/ExtendedSpanProcessorUsageTest.java Outdated Show resolved Hide resolved

trask approved these changes Sep 1, 2024

View reviewed changes

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java Outdated Show resolved Hide resolved

JonasKunz and others added 3 commits September 2, 2024 09:22

Apply suggestions from code review

4aa663c

Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

Make test method package private

fc67a4d

Invert isSpanUnmodifiableByCurrentThread

80aa059

jack-berg merged commit 09de4bd into open-telemetry:main Sep 4, 2024
18 checks passed

mwear mentioned this pull request Sep 13, 2024

Adds on_ending callback to allow processors to mutate spans before End operation open-telemetry/opentelemetry-ruby#1713

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added SpanProcessor OnEnding callback #6367

Added SpanProcessor OnEnding callback #6367

JonasKunz commented Apr 9, 2024 •

edited

Loading

codecov bot commented Aug 7, 2024 •

edited

Loading

jkwatson Aug 13, 2024

JonasKunz Aug 13, 2024 •

edited

Loading

jack-berg left a comment

jkwatson commented Aug 22, 2024

JonasKunz commented Aug 26, 2024

jack-berg commented Aug 28, 2024

jack-berg left a comment

Added SpanProcessor OnEnding callback #6367

Added SpanProcessor OnEnding callback #6367

Conversation

JonasKunz commented Apr 9, 2024 • edited Loading

codecov bot commented Aug 7, 2024 • edited Loading

Codecov Report

jkwatson Aug 13, 2024

Choose a reason for hiding this comment

JonasKunz Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

jack-berg left a comment

Choose a reason for hiding this comment

jkwatson commented Aug 22, 2024

JonasKunz commented Aug 26, 2024

jack-berg commented Aug 28, 2024

jack-berg left a comment

Choose a reason for hiding this comment

JonasKunz commented Apr 9, 2024 •

edited

Loading

codecov bot commented Aug 7, 2024 •

edited

Loading

JonasKunz Aug 13, 2024 •

edited

Loading